chore(cargo workspace): unifies crates as workspace #227
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
needs-rebase
EandrewJones
force-pushed
the
#218-convert-repo-to-rust-workspace
branch
from
38ca8d3 to
0a881f0
Compare
k8s-ci-robot
removed
the
needs-rebase
EandrewJones
force-pushed
the
#218-convert-repo-to-rust-workspace
branch
2 times, most recently
from
6bd43c3 to
0347bcc
Compare
|
Well I wanted to punt on touching the Makefile and Docker builds, but looks like I may have to anyways since the workspace breaks docker image builds. And if I'm going to do it, I may as well do it the right way. |
EandrewJones
force-pushed
the
#218-convert-repo-to-rust-workspace
branch
from
0347bcc to
c4267a4
Compare
k8s-ci-robot
added
size/XL
EandrewJones
force-pushed
the
#218-convert-repo-to-rust-workspace
branch
from
c4267a4 to
54bb634
Compare
|
@astoycos Ready for your review. Apologies for the large PR. Unfortunately, I also had to update the CI, Makefiles, and Dockerfiles to get the test suite passing with the transition to a workspace. |
|
No worries I'm a bit backed up on the TODO list but I will get to this as soon as I can |
service.dataplane.Dockerfile
Outdated
| @@ -21,7 +26,11 @@ RUN if [ "$TARGETARCH" = "amd64" ]; \ | |||
| fi | |||
| RUN rustup target add $(eval cat arch)-unknown-linux-musl | |||
|
|
|||
| COPY . . | |||
| COPY dataplane dataplane | |||
| COPY tools/udp-test-server tools/udp-test-server | |||
There was a problem hiding this comment.
do we need this in the dataplane?
There was a problem hiding this comment.
Unfortunately, we do. If I recall correctly, cargo complains if the referenced workspace members are missing, even if you're only building a single package.
I don't think this is that big of a deal, however, because it doesn't get copied over to the second stage.
If you know a work around, please share.
|
Let's wait for word from Andrew.
Best
Evan Jones
Website: www.ea-jones.com
…On Thu, May 30, 2024 at 7:48 AM Sanskar Jaiswal ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In service.dataplane.Dockerfile
<#227 (comment)>
:
> @@ -32,9 +41,13 @@ RUN --mount=type=cache,target=/workspace/target/ \
cargo xtask build-ebpf --release
RUN --mount=type=cache,target=/workspace/target/ \
--mount=type=cache,target=/root/.cargo/registry \
- RUSTFLAGS=-Ctarget-feature=+crt-static cargo build --release --target=$(eval cat arch)-unknown-linux-musl
+ RUSTFLAGS=-Ctarget-feature=+crt-static cargo build \
+ --workspace \
+ --exclude ebpf \
if the tests are passing, then i must be missing something, please ignore
—
Reply to this email directly, view it on GitHub
<#227 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJ2T6AJXU2T3XIIVMDGBCCDZE4GXPAVCNFSM6AAAAABGOMAFTWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOBYGA2TANZXGA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Just some small things, additionally can you remove https://github.com/kubernetes-sigs/blixt/blob/main/dataplane/ebpf/Cargo.toml#L36 from the ebpf crate's cargo toml it's not a workspace just a standalone crate
You could also document somewhere that the ebpf crate can't be part of the workspace because
error[E0152]: found duplicate lang item `panic_impl`
--> dataplane/ebpf/src/main.rs:117:1
|
117 | / fn panic(_info: &core::panic::PanicInfo) -> ! {
118 | | loop {}
119 | | }
| |_^Or more specifically because the crate needs to re-implements the panic handler
EandrewJones
force-pushed
the
#218-convert-repo-to-rust-workspace
branch
2 times, most recently
from
3899eb0 to
7f967e6
Compare
I added a note in the README. |
Combines all rust crates under a single workspace to simplify management of rust packaging, with the exception of ebpf which needs independent tooling and packages to properly build. Author: Evan Jones <[email protected]>
EandrewJones
force-pushed
the
#218-convert-repo-to-rust-workspace
branch
from
7f967e6 to
8a8157a
Compare
|
/lgtm You're the man @EandrewJones!!! Sorry for my slowness on this one |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: astoycos, EandrewJones The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
No worries. I'm quite buried myself right now, so I appreciated the slowness ;) |
Changes
Combines all rust crates under a single workspace to simplify management of rust packaging.
Closes #218